Handle non-ICRS coordinates in query functions #3164

snbianco · 2024-12-23T20:34:45Z

This PR handles coordinates passed into query functions that are not in the ICRS frame. If it is found that the given coordinates are not in ICRS, they are transformed.

codecov · 2024-12-23T20:40:49Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.27%. Comparing base (43a1b1f) to head (7ebbcf5).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3164      +/-   ##
==========================================
+ Coverage   70.18%   70.27%   +0.09%     
==========================================
  Files         232      232              
  Lines       19937    19973      +36     
==========================================
+ Hits        13992    14037      +45     
+ Misses       5945     5936       -9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bsipocz

I wonder if this should live in the commons function, or you could use what we already have for simbad (_parse_coordinate_and_convert_to_icrs)?
(or also move the simbad one to commons? cc @ManonMarchand )

_parse_coordinate_and_convert_to_icrs

bsipocz · 2025-02-15T04:49:25Z

astroquery/mast/utils.py

@@ -121,6 +121,32 @@ def resolve_object(objectname):
    return coordinates


+def parse_coordinates(coordinates):


I wonder if this should be a kwarg argument for commons.parse_coordinates instead? e.g. to convert right at the end before returning the current output

Do you mean a parameter like return_frame or something to that effect that you could pass in a value like 'ICRS' and get the coordinate back in whatever frame was given?

Just made this change! I added a return_frame parameter to utils.commons.parse_coordinates that defaults to None. If it is provided and the coordinates are not already in that frame, it will attempt to transform them. If the transformation fails, a warning is issued and the coordinates are returned as is, without the transformation.

ManonMarchand · 2025-02-15T09:04:54Z

Oh that methods only parses coordinates if I detect that they are written as a sim-script string. This is an old way of writing coordinates that only existed in the former Simbad interface (hence this is in Simbad's utils and not in commons).
In the other cases I use parse_coordinates from commons and then convert the SkyCoord it returns to icrs 🙂

snbianco · 2025-03-24T20:41:19Z

@bsipocz Pinging this PR again in case it was lost in the shuffle somewhere! I'll be offline from 3/26 - 4/3, but this isn't high priority, so no worries if you don't get around to it before then.

bsipocz

Minor comments only. Thanks!

bsipocz · 2025-07-31T18:51:59Z

astroquery/utils/commons.py

@@ -40,7 +40,7 @@
 ASTROPY_LT_7_1_1 = not minversion('astropy', '7.1.1')


-def parse_coordinates(coordinates):
+def parse_coordinates(coordinates, return_frame=None):


make this mandatory kwarg please

Suggested change

def parse_coordinates(coordinates, return_frame=None):

def parse_coordinates(coordinates, *, return_frame=None):

bsipocz · 2025-07-31T18:53:45Z

astroquery/utils/commons.py

+        except ValueError:
+            warnings.warn(f"Failed to transform coordinates to requested frame: {return_frame}.", InputWarning)


I feel should let the exception propagate, after all this will only be triggered when return frame is explicitly asked for, thus a problem should be surfaced up to the user/module level

bsipocz · 2025-07-31T18:55:12Z

CHANGES.rst

@@ -239,6 +239,8 @@ Infrastructure, Utility and Other Changes and Additions
 - Removed usage of the astropy TestRunner, therefore the unadvertised
  ``astroquery.test()`` functionality. [#3215]

+- ``return_frame`` parameter in ``utils.commons.parse_coordinates`` returns coordinates in the specified frame. [#3164]


please rebase to remove the merge commit and to squash out the changelog iterations, and to put this into the 0.4.11 section. Thanks!

snbianco · 2025-08-01T15:26:42Z

Updated and rebased!

bsipocz

Unfortunately with the most recent mast merges this is now conflicting again. Could you please do a rebase again and make sure that the only diff in the changelog is the line for this PR?

Code looks good, so I'll merge once the rebase is done and it passes the remote tests.

Thanks.

bsipocz · 2025-08-17T05:28:57Z

CHANGES.rst

@@ -14,6 +14,8 @@ gaia

 - Deprecated ``band`` from ``load_data`` as it has no effect on upstream
  response any more. [#3278]
+- New datalink retrieve types EPOCH_PHOTOMETRY_CROWDED_FIELD, EPOCH_ASTROMETRY_BRIGHT, XP_MEAN_SPECTRUM_GRAVLENS,


make sure these are not included in the rebased version

Fix changelog

Small test fix

snbianco · 2025-08-18T19:29:20Z

Rebased! For some reason, the last line in CHANGES.rst is still showing up in the diff. I'm not sure how to fix this since there doesn't seem to be any difference between the two versions.

snbianco · 2025-08-18T19:30:13Z

astroquery/mast/tests/test_mast_remote.py

-
-        # Are the two GALEX observations with obs_id 6374399093149532160 in the results table
-        assert len(result[np.where(result["obs_id"] == "6374399093149532160")]) == 2
-


Removing these assertions as they occasionally break when the number of results increases.

bsipocz

Thanks!

Changing the file ending is weird, but not worth going down the rabbit hole to understand it.

astroquery/mast/tests/test_mast_remote.py

snbianco force-pushed the ASB-29990-coord-frame branch from 4dc257f to f2bd777 Compare January 24, 2025 22:01

snbianco mentioned this pull request Jan 24, 2025

Download Products with MastMissions #3155

Merged

snbianco marked this pull request as ready for review January 24, 2025 22:19

snbianco force-pushed the ASB-29990-coord-frame branch from 4baa044 to 50648fd Compare January 24, 2025 22:25

bsipocz added the mast label Feb 15, 2025

bsipocz added this to the v0.4.10 milestone Feb 15, 2025

bsipocz reviewed Feb 15, 2025

View reviewed changes

snbianco force-pushed the ASB-29990-coord-frame branch from f83c591 to 0746ddf Compare February 21, 2025 19:01

snbianco force-pushed the ASB-29990-coord-frame branch 2 times, most recently from 69ca4b1 to 42597be Compare March 17, 2025 20:54

bsipocz modified the milestones: v0.4.10, v0.4.11 Mar 18, 2025

bsipocz requested changes Jul 31, 2025

View reviewed changes

snbianco force-pushed the ASB-29990-coord-frame branch 3 times, most recently from ef8e8c3 to 43c951c Compare August 1, 2025 14:03

bsipocz reviewed Aug 17, 2025

View reviewed changes

snbianco added 4 commits August 18, 2025 12:12

Utils method to parse coordinates

236cc77

Changes, style, test fixes

0689656

Fix changelog

Move logic into commons

60ed5a3

Fix docs build

743f9bb

Fix changelog

snbianco force-pushed the ASB-29990-coord-frame branch from 873fc23 to 7ebbcf5 Compare August 18, 2025 16:17

Make mandatory kwarg, propagate exception

12425e2

Small test fix

snbianco force-pushed the ASB-29990-coord-frame branch from d2befba to 12425e2 Compare August 18, 2025 19:28

snbianco commented Aug 18, 2025

View reviewed changes

bsipocz approved these changes Aug 18, 2025

View reviewed changes

bsipocz reviewed Aug 18, 2025

View reviewed changes

astroquery/mast/tests/test_mast_remote.py Show resolved Hide resolved

bsipocz merged commit 01939a4 into astropy:main Aug 18, 2025
10 checks passed

		@@ -121,6 +121,32 @@ def resolve_object(objectname):
		return coordinates


		def parse_coordinates(coordinates):

	def parse_coordinates(coordinates, return_frame=None):
	def parse_coordinates(coordinates, *, return_frame=None):

		except ValueError:
		warnings.warn(f"Failed to transform coordinates to requested frame: {return_frame}.", InputWarning)


		# Are the two GALEX observations with obs_id 6374399093149532160 in the results table
		assert len(result[np.where(result["obs_id"] == "6374399093149532160")]) == 2

Uh oh!

Handle non-ICRS coordinates in query functions #3164

Handle non-ICRS coordinates in query functions #3164

Uh oh!

Conversation

snbianco commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManonMarchand commented Feb 15, 2025

Uh oh!

snbianco commented Mar 24, 2025

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snbianco commented Aug 1, 2025

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snbianco commented Aug 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snbianco commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading